-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
Allow fillna(value=None, method="constant")
#28124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hello @valtron! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2019-08-24 05:29:33 UTC |
|
I think I'm -1 on this as it adds complexity for a rather niche case. What real advantage are you hoping to get out of replacing NA with None? |
|
I'm also initially against this. We have a few things to work out with NA values (#28095). It's not clear how None will be handled there. If we wanted to support this, the easier way would be If the user passes None, it'd be up to the dtype to determine whether or not that's a valid fill value. But we'll need to settle on whether this is desirable before moving forward. @valtron if you have use cases where |
|
@WillAyd I've had BTW, currently @TomAugspurger I tried implementing it that way (using a sentinel for the default value) at first, but it requires more changes, and wouldn't be backward-compatible (e.g. |
Can you expand on what "more changes" entails? Understood concern on the latter piece but generally this seems like a more ideal approach to get what you are after |
|
I spent a bit more time on the alternate implementation (https://github.com/valtron/pandas/commit/66e987faca4a677928479e402302ddefb5099398); I don't have all the tests passing yet, but so far I'm guessing the remaining changes would all involve tracking down |
|
Thanks for the contribution! I think this is a hold for now though, and as mentioned before probably worth voicing thoughts in #28095 to support this feature |
Sometimes data has object columns that contain both
NaNs andNoneand we want to standardize them toNone. Currentlyfillnadoesn't allowvalue=NonebecauseNoneis used to mean "no value", andmethod=Noneto mean "fill with the value", so when both are none it's considered invalid. (On columns with dtypes that have their own NAs, like floats, timestamps, etc., filling withNoneleaves them as-is.)To get around this, I added
method="constant"which should also be taken to mean "fill with the value".method="constant"is required if filling withNone(so thatvalue=None, method=Nonestill throws), otherwise it works the same as before.As far as I can tell, doing it this way shouldn't break any existing code. I updated/added some tests for the new behaviour. If it looks like this change might be accepted, I'll update the docs as well.
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff